OSASINFRA-3133 - OpenStack support by EmilienM · Pull Request #195 · openshift/cluster-control-plane-machine-set-operator

EmilienM · 2023-04-14T17:10:59Z

OpenStack support in CPMS.

openshift-ci · 2023-04-14T17:11:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go

odvarkadaniel

Generally, the code LGTM. I would appreciate that someone else would look at this after KubeCon.

cc/ @JoelSpeed @damdo

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go

test/e2e/framework/framework.go

pkg/controllers/controlplanemachinesetgenerator/controller_test.go

pkg/controllers/controlplanemachinesetgenerator/openstack.go

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go

EmilienM · 2023-06-22T19:49:54Z

/test unit

EmilienM · 2023-06-22T20:00:24Z

/test e2e-azure-operator e2e-aws-ovn-serial e2e-aws-ovn

EmilienM · 2023-06-22T20:07:51Z

pkg/controllers/controlplanemachinesetgenerator/aws.go


 // buildAWSFailureDomains builds an AWS flavored FailureDomains for the ControlPlaneMachineSet.
-func buildAWSFailureDomains(failureDomains *failuredomain.Set) machinev1.FailureDomains {
+func buildAWSFailureDomains(failureDomains *failuredomain.Set) (machinev1.FailureDomains, error) { //nolint:unparam


note for reviewers: in a comment with Damiano, we want consistent buildFD functions for the providers, so since OpenStack can return an actual error, we let all builders to return an error too (even if the error is nil for now).

EmilienM · 2023-06-28T15:31:13Z

/test unit

EmilienM · 2023-06-28T19:14:21Z

I've just spotted an issue when testing this again, but I'm not sure if it's only in my environment or what:

  Template:
    Machine Type:  machines_v1beta1_machine_openshift_io
    machines_v1beta1_machine_openshift_io:
      Failure Domains:
        Openstack:
          Availability Zone:  az0
          Availability Zone:  az1
          Availability Zone:  az2
        Platform:             OpenStack
      Metadata:
        Labels:
          machine.openshift.io/cluster-api-cluster:       refarch-b4dr4
          machine.openshift.io/cluster-api-machine-role:  master
          machine.openshift.io/cluster-api-machine-type:  master
      Spec:
        Lifecycle Hooks:
        Metadata:
        Provider Spec:
          Value:
            API Version:        machine.openshift.io/v1alpha1
            Availability Zone:  az2
            Cloud Name:         openstack
            Clouds Secret:
              Name:       openstack-cloud-credentials
              Namespace:  openshift-machine-api
            Flavor:       m1.xlarge
            Image:        rhcos-4.14
            Kind:         OpenstackProviderSpec
            Metadata:
              Creation Timestamp:  <nil>
            Networks:
              Filter:
              Subnets:
                Filter:
                  Name:  refarch-b4dr4-nodes
                  Tags:  openshiftClusterID=refarch-b4dr4
            Security Groups:
              Filter:
              Name:             refarch-b4dr4-master
            Server Group Name:  refarch-b4dr4-master
            Server Metadata:
              Name:                  refarch-b4dr4-master
              Openshift Cluster ID:  refarch-b4dr4
            Tags:
              openshiftClusterID=refarch-b4dr4
            Trunk:  true
            User Data Secret:
              Name:  master-user-data

The Provider Spec shouldn't have the Availability Zone field; as it's part of the Failure Domains.

pkg/controllers/controlplanemachinesetgenerator/openstack.go

pierreprinetti · 2023-06-28T21:00:15Z

The Provider Spec shouldn't have the Availability Zone field; as it's part of the Failure Domains.

I am interested what you think should be in the providerSpec’s availabilityZone. The empty string?

EmilienM · 2023-06-28T21:10:10Z

The Provider Spec shouldn't have the Availability Zone field; as it's part of the Failure Domains.

I am interested what you think should be in the providerSpec’s availabilityZone. The empty string?

I might clarify what I said but I'm talking about the providerSpec in the CPMS:

oc describe controlplanemachineset.machine.openshift.io cluster --namespace openshift-machine-api
Name:         cluster
Namespace:    openshift-machine-api
Labels:       machine.openshift.io/cluster-api-cluster=refarch-vgnhs
Annotations:  <none>
API Version:  machine.openshift.io/v1
Kind:         ControlPlaneMachineSet
Metadata:
  Creation Timestamp:  2023-06-28T20:17:38Z
  Finalizers:
    controlplanemachineset.machine.openshift.io
  Generation:  1
  Managed Fields:
    API Version:  machine.openshift.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          .:
          f:machine.openshift.io/cluster-api-cluster:
      f:spec:
        .:
        f:replicas:
        f:selector:
        f:state:
        f:strategy:
          .:
          f:type:
        f:template:
          .:
          f:machineType:
          f:machines_v1beta1_machine_openshift_io:
            .:
            f:failureDomains:
              .:
              f:openstack:
              f:platform:
            f:metadata:
              .:
              f:labels:
                .:
                f:machine.openshift.io/cluster-api-cluster:
                f:machine.openshift.io/cluster-api-machine-role:
                f:machine.openshift.io/cluster-api-machine-type:
            f:spec:
              .:
              f:lifecycleHooks:
              f:metadata:
              f:providerSpec:
                .:
                f:value:
                  .:
                  f:apiVersion:
                  f:availabilityZone:
                  f:cloudName:
                  f:cloudsSecret:
                    .:
                    f:name:
                    f:namespace:
                  f:flavor:
                  f:image:
                  f:kind:
                  f:metadata:
                    .:
                    f:creationTimestamp:
                  f:networks:
                  f:securityGroups:
                  f:serverGroupName:
                  f:serverMetadata:
                    .:
                    f:Name:
                    f:openshiftClusterID:
                  f:tags:
                  f:trunk:
                  f:userDataSecret:
                    .:
                    f:name:
    Manager:      cluster-bootstrap
    Operation:    Update
    Time:         2023-06-28T20:17:38Z
    API Version:  machine.openshift.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
    Manager:      cluster-bootstrap
    Operation:    Update
    Subresource:  status
    Time:         2023-06-28T20:17:38Z
    API Version:  machine.openshift.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"controlplanemachineset.machine.openshift.io":
    Manager:      Go-http-client
    Operation:    Update
    Time:         2023-06-28T20:25:02Z
    API Version:  machine.openshift.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        f:conditions:
          .:
          k:{"type":"Available"}:
            .:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
            f:type:
          k:{"type":"Degraded"}:
            .:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
            f:type:
          k:{"type":"Error"}:
            .:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
            f:type:
          k:{"type":"Progressing"}:
            .:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
            f:type:
        f:observedGeneration:
        f:readyReplicas:
        f:replicas:
        f:updatedReplicas:
    Manager:         Go-http-client
    Operation:       Update
    Subresource:     status
    Time:            2023-06-28T20:28:53Z
  Resource Version:  17954
  UID:               00f6f58f-22b3-47c6-80c7-948c6b0fe369
Spec:
  Replicas:  3
  Selector:
    Match Labels:
      machine.openshift.io/cluster-api-cluster:       refarch-vgnhs
      machine.openshift.io/cluster-api-machine-role:  master
      machine.openshift.io/cluster-api-machine-type:  master
  State:                                              Active
  Strategy:
    Type:  RollingUpdate
  Template:
    Machine Type:  machines_v1beta1_machine_openshift_io
    machines_v1beta1_machine_openshift_io:
      Failure Domains:
        Openstack:
          Availability Zone:  az0
          Availability Zone:  az1
          Availability Zone:  az2
        Platform:             OpenStack
      Metadata:
        Labels:
          machine.openshift.io/cluster-api-cluster:       refarch-vgnhs
          machine.openshift.io/cluster-api-machine-role:  master
          machine.openshift.io/cluster-api-machine-type:  master
      Spec:
        Lifecycle Hooks:
        Metadata:
        Provider Spec:
          Value:
            API Version:        machine.openshift.io/v1alpha1
            Availability Zone:  az2
            Cloud Name:         openstack
            Clouds Secret:
              Name:       openstack-cloud-credentials
              Namespace:  openshift-machine-api
            Flavor:       m1.xlarge
            Image:        refarch-vgnhs-rhcos
            Kind:         OpenstackProviderSpec
            Metadata:
              Creation Timestamp:  <nil>
            Networks:
              Filter:
              Subnets:
                Filter:
                  Name:  refarch-vgnhs-nodes
                  Tags:  openshiftClusterID=refarch-vgnhs
            Security Groups:
              Filter:
              Name:             refarch-vgnhs-master
            Server Group Name:  refarch-vgnhs-master
            Server Metadata:
              Name:                  refarch-vgnhs-master
              Openshift Cluster ID:  refarch-vgnhs
            Tags:
              openshiftClusterID=refarch-vgnhs
            Trunk:  true
            User Data Secret:
              Name:  master-user-data

It should not be there, AFIK.
At least it wasn't there a few weeks ago when I did the first demo of this thing working.

JoelSpeed · 2023-06-29T08:29:31Z

The Provider Spec shouldn't have the Availability Zone field; as it's part of the Failure Domains.

I am interested what you think should be in the providerSpec’s availabilityZone. The empty string?

Within the CPMS providerSpec, anything that's meant to be set by the failure domain, should be cleared, so yes, empty string

EmilienM · 2023-06-29T14:53:08Z

/test e2e-openstack-operator

EmilienM · 2023-06-30T01:54:17Z

@pierreprinetti much better with the new installer patchset:

Spec:
  Replicas:  3
  Selector:
    Match Labels:
      machine.openshift.io/cluster-api-cluster:       refarch-s26qd
      machine.openshift.io/cluster-api-machine-role:  master
      machine.openshift.io/cluster-api-machine-type:  master
  State:                                              Active
  Strategy:
    Type:  RollingUpdate
  Template:
    Machine Type:  machines_v1beta1_machine_openshift_io
    machines_v1beta1_machine_openshift_io:
      Failure Domains:
        Openstack:
          Availability Zone:  az0
          Availability Zone:  az1
          Availability Zone:  az2
        Platform:             OpenStack
      Metadata:
        Labels:
          machine.openshift.io/cluster-api-cluster:       refarch-s26qd
          machine.openshift.io/cluster-api-machine-role:  master
          machine.openshift.io/cluster-api-machine-type:  master
      Spec:
        Lifecycle Hooks:
        Metadata:
        Provider Spec:
          Value:
            API Version:  machine.openshift.io/v1alpha1
            Cloud Name:   openstack
            Clouds Secret:
              Name:       openstack-cloud-credentials
              Namespace:  openshift-machine-api
            Flavor:       m1.xlarge
            Image:        refarch-s26qd-rhcos
            Kind:         OpenstackProviderSpec
            Metadata:
              Creation Timestamp:  <nil>
            Networks:
              Filter:
              Subnets:
                Filter:
                  Name:  refarch-s26qd-nodes
                  Tags:  openshiftClusterID=refarch-s26qd
            Security Groups:
              Filter:
              Name:             refarch-s26qd-master
            Server Group Name:  refarch-s26qd-master
            Server Metadata:
              Name:                  refarch-s26qd-master
              Openshift Cluster ID:  refarch-s26qd
            Tags:
              openshiftClusterID=refarch-s26qd
            Trunk:  true
            User Data Secret:
              Name:  master-user-data

@damdo @JoelSpeed ready for review when you have time.
Let me know if you want me to squash the commits.

Nothing changed much since your last reviews, except the last 2 commits I would say, which are small enough to be quickly reviewed.

Co-Authored-By: Emilien Macchi <emilien@redhat.com> Co-Authored-By: Pierre Prinetti <pierreprinetti@redhat.com>

test/e2e/helpers/machine.go

If `OPENSTACK_CONTROLPLANE_FLAVOR_ALTERNATE` is found in the environment variables, we'll use that flavor when running the control plane verticale scale tests. Otherwise, roll back on the tags.

JoelSpeed · 2023-06-30T12:49:55Z

docs/user/failure-domains.md

+```yaml
+- availabilityZone: "<nova availability zone>"
+  rootVolume:
+    availabilityZone: "<cinder availability zone>"


Didn't we add another field to the AZ?

if you're talking about the volume type, it'll be handled separately via #217

JoelSpeed · 2023-06-30T12:50:14Z

docs/user/installation.md

+
+#### Configuring a control plane machine set on OpenStack
+
+Two fields are supported for now: `availabilityZone` (instance AZ) and `rootVolume.availabilityZone` (root volume AZ).


Does this need updating for the third field that we added?

volume type will be handled separately via #217

pkg/controllers/controlplanemachinesetgenerator/utils.go

JoelSpeed · 2023-06-30T13:53:30Z

Apart from the nit about the errors, I think this is good to go, thanks for working with us on this 😄

JoelSpeed · 2023-06-30T13:56:06Z

/approve

pierreprinetti

This code seems to be behaving conform to the spec. I believe that there is no reason to further defer testing in vivo.

openshift-ci · 2023-07-03T14:53:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, pierreprinetti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pierreprinetti · 2023-07-03T14:58:15Z

/lgtm

pierreprinetti · 2023-07-03T15:04:41Z

/retest-required

damdo

LGTM
Thanks for all you efforts!

openshift-ci-robot · 2023-07-03T17:14:39Z

/retest-required

Remaining retests: 0 against base HEAD 88ec3eb and 2 for PR HEAD 464c8dc in total

openshift-ci · 2023-07-03T18:35:12Z

@EmilienM: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-azure-ovn-etcd-scaling	`464c8dc`	link	false	`/test e2e-azure-ovn-etcd-scaling`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pierreprinetti · 2023-07-03T18:40:58Z

/retest-required

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2023

EmilienM force-pushed the openstack branch 9 times, most recently from 2add629 to 3c9d625 Compare April 18, 2023 14:55

EmilienM commented Apr 18, 2023

View reviewed changes

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go Outdated Show resolved Hide resolved

EmilienM force-pushed the openstack branch 7 times, most recently from 41d0792 to ca2234e Compare April 19, 2023 18:49

EmilienM marked this pull request as ready for review April 19, 2023 19:11

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2023

openshift-ci bot requested review from elmiko and odvarkadaniel April 19, 2023 19:12

odvarkadaniel reviewed Apr 20, 2023

View reviewed changes

pierreprinetti reviewed Apr 20, 2023

View reviewed changes

pkg/controllers/controlplanemachinesetgenerator/openstack.go Outdated Show resolved Hide resolved

pierreprinetti reviewed Apr 20, 2023

View reviewed changes

pkg/controllers/controlplanemachinesetgenerator/openstack.go Outdated Show resolved Hide resolved

pierreprinetti reviewed Apr 20, 2023

View reviewed changes

pkg/controllers/controlplanemachinesetgenerator/openstack.go Outdated Show resolved Hide resolved

pierreprinetti reviewed Apr 20, 2023

View reviewed changes

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go Outdated Show resolved Hide resolved

EmilienM force-pushed the openstack branch from ca2234e to 1abd3dc Compare April 20, 2023 15:29

EmilienM commented Jun 22, 2023

View reviewed changes

pierreprinetti mentioned this pull request Jun 28, 2023

OSASINFRA-3191: Vertically scale masters with CPMS openshift/release#40751

Closed

1 task

EmilienM mentioned this pull request Jun 28, 2023

OCPBUGS-15338: Fix flaking machine mapping testing #220

Merged

vendoring update for OpenStack CPMS

448ac5f

EmilienM commented Jun 28, 2023

View reviewed changes

pkg/controllers/controlplanemachinesetgenerator/openstack.go Show resolved Hide resolved

EmilienM and others added 2 commits June 30, 2023 14:50

OpenStack support in CPMS

c5397e2

Co-Authored-By: Emilien Macchi <emilien@redhat.com> Co-Authored-By: Pierre Prinetti <pierreprinetti@redhat.com>

Add support for no failure domains in OpenStack platform

afbf7c8

pierreprinetti reviewed Jun 30, 2023

View reviewed changes

test/e2e/helpers/machine.go Outdated Show resolved Hide resolved

e2e: allow to run regular vertical scale tests in OpenStack

464c8dc

If `OPENSTACK_CONTROLPLANE_FLAVOR_ALTERNATE` is found in the environment variables, we'll use that flavor when running the control plane verticale scale tests. Otherwise, roll back on the tags.

JoelSpeed reviewed Jun 30, 2023

View reviewed changes

EmilienM mentioned this pull request Jun 30, 2023

OSASINFRA-3155 - OpenStack: Create ControlPlaneMachineSet CRDs openshift/installer#7280

Merged

pierreprinetti approved these changes Jul 3, 2023

View reviewed changes

damdo reviewed Jul 3, 2023

View reviewed changes


		#### Configuring a control plane machine set on OpenStack

		Two fields are supported for now: `availabilityZone` (instance AZ) and `rootVolume.availabilityZone` (root volume AZ).

Conversation

EmilienM commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Apr 14, 2023

Uh oh!

Uh oh!

odvarkadaniel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmilienM commented Jun 22, 2023

Uh oh!

EmilienM commented Jun 22, 2023

Uh oh!

EmilienM Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Jun 28, 2023

Uh oh!

EmilienM commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pierreprinetti commented Jun 28, 2023

Uh oh!

EmilienM commented Jun 28, 2023

Uh oh!

JoelSpeed commented Jun 29, 2023

Uh oh!

EmilienM commented Jun 29, 2023

Uh oh!

EmilienM commented Jun 30, 2023

Uh oh!

Uh oh!

JoelSpeed Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

EmilienM Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

EmilienM Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed commented Jun 30, 2023

Uh oh!

JoelSpeed commented Jun 30, 2023

Uh oh!

pierreprinetti left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 3, 2023

Uh oh!

pierreprinetti commented Jul 3, 2023

Uh oh!

pierreprinetti commented Jul 3, 2023

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 3, 2023

Uh oh!

openshift-ci bot commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierreprinetti commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

EmilienM commented Apr 14, 2023 •

edited

Loading

EmilienM commented Jun 28, 2023 •

edited

Loading

openshift-ci bot commented Jul 3, 2023 •

edited

Loading